Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪛 Moving to SQLAlchemy 2.0 #540

Merged
merged 10 commits into from
Feb 21, 2024
Merged

Conversation

tarsil
Copy link
Contributor

@tarsil tarsil commented Mar 22, 2023

  • Added support for python 3.11
  • Added common and dialects packages to handle the new SQLAlchemy 2.0+

@aminalaee @Kludex this is a cleaner version of the previous PR with the changes passing the new SQLAlchemy 2.0 which is significantly different from the < 2.0

It is also good to mention that when I was doing this PR, I was reading some suggestions and the one from here #507 was good to add it as well. So I would like to thank @circlingthesun and @JGoutin for the suggestions

Edit: The asyncpg sometimes hangs during the tests. I forcibly close the connections during that test to make sure it doesn't get stuck

* Removed support for python 3.7
* Added common and dialects packages to handle
the new SQLAlchemy 2.0+
* The connections were hanging sometimes
during the tests.
@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

@aminalaee @Kludex maybe you could manually kill:

Please? This was the whole reason why the second commit. Quite a well known issue but that CI passed with flying colours, only these 2 are hanging

@tarsil tarsil changed the title 🪛 Added support for SQLAlchemy 2.0 🪛 Moving to SQLAlchemy 2.0 Mar 22, 2023
@tarsil tarsil mentioned this pull request Mar 22, 2023
@Kludex
Copy link
Member

Kludex commented Mar 22, 2023

Done

@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

Well, I hope this is ok. Quite nice and clean integration for SQLAlchemy 2.0

@lucasguiss
Copy link

I've forked the version of this PR and it's working fine in our apps, waiting for the release 😄

@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

@lucasguiss very happy to hear that :)

@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

@Kludex here the status is not "complete" because this PR also makes sure python 3.7 is no longer supported for many reasons but also in June the support will stop.

@Kludex
Copy link
Member

Kludex commented Mar 22, 2023

@Kludex here the status is not "complete" because this PR also makes sure python 3.7 is no longer supported for many reasons but also in June the support will stop.

Why did you remove now?

@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

@Kludex here the status is not "complete" because this PR also makes sure python 3.7 is no longer supported for many reasons but also in June the support will stop.

Why did you remove now?

Well, I can always add and we remove after june, I would say. The initial tests were failing with some 3.7 issues but I will add it back. I think I solved them

@Kludex
Copy link
Member

Kludex commented Mar 22, 2023

Can we remove all changes that are not related to the support of SQLAlchemy 2.0? You can create other PRs for them, but let's focus on a single thing here, please.

@tarsil
Copy link
Contributor Author

tarsil commented Mar 22, 2023

Can we remove all changes that are not related to the support of SQLAlchemy 2.0? You can create other PRs for them, but let's focus on a single thing here, please.

@Kludex Done. It was mainly that, had initial configuration issues with python 3.7 but that was addressed.

The changes you see are the ones used for the move to SQLAlchemy 2.0+

Regarding other PRs. I can do that later once the time comes. This was already a good change that allow us to move forward.

@Kludex Kludex requested a review from aminalaee March 22, 2023 22:16
CHANGELOG.md Outdated Show resolved Hide resolved
.github/workflows/test-suite.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
docs/index.md Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
* Revert changes automatically applied by IDE
* Fix formatting of CI caused by IDE changes
* Codebase cleanup for SQLAlchemy 2.0
* Fix EOL in mkdocs
@tarsil tarsil force-pushed the feature/add_sqlalchemy_2 branch 2 times, most recently from 23223c9 to b271a54 Compare March 24, 2023 10:31
@tarsil
Copy link
Contributor Author

tarsil commented Mar 26, 2023

Hey @aminalaee . Any news about this? I know you are extremely busy

@aminalaee
Copy link
Member

Hi @tarsil , I will take a look today or tomorrow. Sorry for the delay.

@mathause
Copy link

I think you'll have to update setup.py as well:

install_requires=["sqlalchemy>=1.4.42,<1.5"],

@tarsil
Copy link
Contributor Author

tarsil commented Mar 26, 2023

I think you'll have to update setup.py as well:

install_requires=["sqlalchemy>=1.4.42,<1.5"],

I did update it as well before. Its there 👍🏼

@mathause
Copy link

pip complained when I installed the branch, but I see it now. Not sure why I missed it before - sorry for the noise.

This was referenced Mar 27, 2023
@tarsil
Copy link
Contributor Author

tarsil commented Mar 30, 2023

@aminalaee sorry to a bother but, any news?

@tarsil
Copy link
Contributor Author

tarsil commented Apr 27, 2023

@Kludex this ia probably my fault. Since our last conversation unfortunately I didn't have enough time to address these comments. @rafalp in your code comment. I know SQLA stopped supporting that and that is the reason why I didn't change too much and I simple added the * as per normal python.

I promise I will try to address all the comments soon. Apologies

@MAKMED1337
Copy link

MAKMED1337 commented Apr 30, 2023

Few days ago SQLAlchemy released 2.0.11 where they have changed Row class
Will it be supported ?

return raw

def __iter__(self) -> typing.Iterator:
return iter(self._row.keys())
Copy link

@MAKMED1337 MAKMED1337 Apr 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of this you can't unpack result anymore
Example:

class LastUsername(Base):
	__tablename__ = 'last_username'
	user_id = Column(BIGINT, primary_key=True)
	username = Column(TEXT, nullable=False)

r = await db.fetch_one(select(LastUsername))
user_id, username = r
assert user_id == 'user_id' #but it need to be at least int

@stonebig
Copy link

stonebig commented Jun 4, 2023

hum, any hope for this to be merged ?

@Kludex
Copy link
Member

Kludex commented Jun 4, 2023

hum, any hope for this to be merged ?

If comments are addressed or answered, we can continue here

@Feijo
Copy link

Feijo commented Jun 4, 2023

Honestly I just decided to give up on this library altogether and use the new async features built-in into SA2.

@stlomib
Copy link

stlomib commented Jul 6, 2023

Any update?

1 similar comment
@POLIROID
Copy link

POLIROID commented Aug 8, 2023

Any update?

@tarsil
Copy link
Contributor Author

tarsil commented Aug 8, 2023

Sorry, didn't have enough time to fully address this but I will soon

@philipbjorge
Copy link

@tarsil --
Thanks for putting this together --
We've put this in one of our projects and our test suite was 🟢.

We'll be kicking the tires more on this over the coming weeks and sharing any feedback or discoveries here.

(We did pin on SA 2.0.10 based on this comment)

@ziglef
Copy link

ziglef commented Nov 28, 2023

Any updates or roadmap for this PR?

@philipbjorge
Copy link

We just shipped this branch out to production and haven't noticed any problems yet.
We're a fairly low volume service, but overall feeling very good about this branch.
We're using this as a bridge while we port over to SA2.

@adnam
Copy link

adnam commented Dec 5, 2023

I've tried out this branch by specifying the following in my requirements file:

databases @ git+https://github.com/tarsil/databases@feature/add_sqlalchemy_2

which resulted in pip-compile choosing sqlalchemy==2.0.23 - the latest version as of Nov 2 '23.

As previously mentioned here the Row class has changed in v2.0.11 resulting in an error like this when performing a query:

AttributeError: type object 'Row' has no attribute '_default_key_style'

I think it would be worth updating this PR to account for the new Row class before merging, otherwise projects using databases will be pinned to an sqlalchemy version from April '23.

@tarsil I'm not very familiar with SqlAlchemy internals but the solution would be something like this: adnam@96e15fd

@pmdevita
Copy link
Contributor

@ansipunk solved this here tarsil#2 Just need to get this merged and we'll be back to 100% tests passing.
What else needs to be done for this PR to be merged?

Fix compatibility with SQLAlchemy>=2.0.11
@tarsil
Copy link
Contributor Author

tarsil commented Feb 20, 2024

@ansipunk solved this here tarsil#2 Just need to get this merged and we'll be back to 100% tests passing. What else needs to be done for this PR to be merged?

It is merged and I do apologize for not receiving notifications and missing this.

@Kludex
Copy link
Member

Kludex commented Feb 21, 2024

When @tarsil is happy (and this is ready to be merged...), I'll merge it and make a release. Ping me.

@tarsil
Copy link
Contributor Author

tarsil commented Feb 21, 2024

@Kludex i do believe I just need to fix the conflicts and we should be good to go

@Kludex
Copy link
Member

Kludex commented Feb 21, 2024

I've invited you to encode @tarsil . Ask me on PM if you have questions about workflows or anything.

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge when happy @tarsil 👍

@tarsil tarsil merged commit 1e40ad1 into encode:master Feb 21, 2024
5 checks passed
@tarsil tarsil deleted the feature/add_sqlalchemy_2 branch February 21, 2024 12:26
@tarsil
Copy link
Contributor Author

tarsil commented Feb 21, 2024

@Kludex , I just merged after fixing the minor conflicts (normal as it was an old branch) and passed 👍🏼

@tarsil
Copy link
Contributor Author

tarsil commented Feb 21, 2024

I've invited you to encode @tarsil . Ask me on PM if you have questions about workflows or anything.

Will do but so far was just this simple merge conflict, I believe. Thank you for the invite.

@ansipunk ansipunk mentioned this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.